Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't pass empty lists of certificates to the oVirt SDK #14160

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Don't pass empty lists of certificates to the oVirt SDK #14160

merged 1 commit into from
Mar 14, 2017

Conversation

jhernand
Copy link
Contributor

@jhernand jhernand commented Mar 3, 2017

The oVirt SDK accepts lists of trusted CA certificates. But when an
empty list, or a list containing just 'nil' elements is passed, the
result is that no certificate is trusted, not even the system default
trusted certificate. To avoid that this patch changes the oVirt provider
so that it passes 'nil' instead of empty lists.

The oVirt SDK accepts lists of trusted CA certificates. But when an
empty list, or a list containing just 'nil' elements is passed, the
result is that no certificate is trusted, not even the system default
trusted certificate. To avoid that this patch changes the oVirt provider
so that it passes 'nil' instead of empty lists.

Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
@jhernand
Copy link
Contributor Author

jhernand commented Mar 3, 2017

@miq-bot assign @oourfali

@borod108 @pkliczewski please review.

@miq-bot
Copy link
Member

miq-bot commented Mar 3, 2017

Checked commit https://github.com/jhernand/manageiq/commit/118b7ec52fb9336d7b7af59115da6442fd4db1a2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍰

@jhernand
Copy link
Contributor Author

jhernand commented Mar 7, 2017

@durandom please review.

@chessbyte
Copy link
Member

@oourfali the changes in this PR looks trivial. Please give your 👍 so we can merge.

@oourfali
Copy link

oourfali commented Mar 8, 2017

Looks good to me.

# values, so we need to check the value passed and make a list only if it won't be empty. If it will be empty then
# we should just pass 'nil'.
ca_certs = options[:ca_certs]
ca_certs = [ca_certs] if ca_certs
Copy link
Member

@durandom durandom Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so options[:ca_certs] is not an array already? Just read the code and it seems to be a concatenated String of certs... well :)

:ca_certs => options[:ca_certs].present? ? Array(options[:ca_certs]) : nil

because

[8] pry(main)> Array(nil)
=> []
[9] pry(main)> Array([])
=> []
[10] pry(main)> [].present?
=> false
[11] pry(main)> ''.present?
=> false
[12] pry(main)> nil.present?
=> false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options[:ca_certs] is the content of the endpoints.certificate_authority column, and that can be nil or a string containing one or multiple concatenated certificates. The SDK accepts a list, to makes things simple for users that may have those multiple certificates already split into multiple strings. Each item of that list can also contain multiple certificates concatenated in a single string. The only problem is that when the list [] or [nil] the behaviour of the SDK is to trust no certificate, which is not what we want, what we want in that case is to trust the default system certificates. The way to achieve that in hte SDK is to pass nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhernand can options[:ca_certs] have more than one cert? It is plural so it sounds like it :)
In that case you'd end up with [[cert1, cert2]], how about ca_certs = Array(ca_certs) if ca_certs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options[:ca_certs] is the content of the endpoints.certificate_authority column. That is a single string that may contain multiple concatenated certificates, that is why the name is plural, but the content is either nil or one string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if you've already covered this but if it is a comma separated string should you split it up here? It looks like OvirtSDK4::Connection is expecting an array of strings https://github.com/oVirt/ovirt-engine-sdk-ruby/blob/master/sdk/lib/ovirtsdk4/connection.rb#L132-L136

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. The content of that database is one string contaning one ore more certificates, in PEM format. For example:

-----BEGIN CERTIFICATE----
The first certificate.
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
The second certificate.
-----END CERTIFICATE-----
...

For user convenience, the oVirt SDK accepts them in separate strings, which would require us to split them here, but not using command but the BEGIN/END marks. Fortunately, also for user conveninence, the oVirt SDK also accepts multiple certificates in the same string, so we don't need to split anything here. The only thing that the SDK doesn't accept is [nil] (well, it accepts it, but it doesn't have the meaning that we need: it trusts no CA instead of trusting any CA), so that is the only case that we need to check here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it thanks :)

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me

@jhernand your version is good too - mine is a matter of taste and has a bit more safeguards

@chessbyte chessbyte assigned blomquisg and unassigned oourfali Mar 8, 2017
@durandom
Copy link
Member

@miq-bot assign @agrare

@agrare pls merge

@miq-bot miq-bot assigned agrare and unassigned blomquisg Mar 14, 2017
@agrare agrare merged commit 5653c37 into ManageIQ:master Mar 14, 2017
@agrare agrare added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 14, 2017
@jhernand
Copy link
Contributor Author

Thanks @agrare.

@jhernand jhernand deleted the dont_pass_empty_cert_list_to_ovirt_sdk branch March 14, 2017 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants